Modernize ES5 to ES6 with lebab #607

rattrayalex-stripe · 2019-05-04T03:54:05Z

Builds on #604, targeting the v7.0.0 release

Uses https://github.com/lebab/lebab, running each transform in its own commit.

See the script I added in the first commit (and removed in the last) for the process.

The primary benefits are, IMO, var->const, function->=> and {foo: foo}->{foo}.

cc @stripe/api-libraries
cc @paulasjes-stripe @robz-stripe @ob-stripe

I'm putting a lot of faith in the test suite here, and my confidence isn't 100% that this is bug-free. I'm not totally sure how to proceed with confidence, or who should review.

EDIT: looking through these commit-by-commit, a few I'm reverting because I think they're bad/dangerous.

These I'm not sure about:

c1934de I think I should just carefully check that there's code coverage for any possible bugs introduced here. I like the change and would like to keep it.
08e4905 Relatedly, I love this change but do think there's some possibility it could introduce some subtle edge-case bugs. This one however is way too big to audit. I think any bugs that the test suite wouldn't have caught would be of the shape of "a user customizes their stripe-node setup in some way that we support but don't test and there was some implicit reliance on this". I'm not sure how large the surface area for this is...

Note that I made a few changes by hand (which I probably should have done in separate commits in retrospect). They were:

This needed to stay function instead of arrow functions:

stripe-node/lib/ResourceNamespace.js

Lines 16 to 20 in d80c8f5

    
           module.exports = function(namespace, resources) { 
        
             return function(stripe) { 
        
               return new ResourceNamespace(stripe, resources); 
        
             }; 
        
           };

These needed to move to const (lebab moved var to let, then eslint complained that they were not reassigned):

stripe-node/lib/StripeResource.js

Lines 341 to 343 in d80c8f5

    
           const apiVersion = self._stripe.getApiField('version'); 
        
           requestData = data; 
        
           const headers = self._defaultHeaders(

This needed to stay a function:

stripe-node/test/utils.spec.js

Line 114 in d80c8f5

constructor: function() {

rattrayalex-stripe · 2019-05-04T04:16:47Z

cc @irace-stripe FYI – looks like it should mostly affect you in that it translates var to const at the top of the files.

paulasjes-stripe · 2019-05-06T13:29:13Z

I ran the tests on node v6.0.0 and v8.0.0. Works as expected in v6, broke on autopagination tests in v8. Turns out it's a node error, upgrading to v8.1 fixed the issue.

Perhaps we should exclude v8 in favour of v8.1.0?

irace-stripe · 2019-05-06T14:06:06Z

cc @irace-stripe FYI – looks like it should mostly affect you in that it translates var to const at the top of the files.

Noted – a change I’m happy to make!

paulasjes-stripe · 2019-05-06T15:38:21Z

lib/StripeResource.js

@@ -50,7 +50,7 @@ StripeResource.prototype = {
  // Methods that don't use the API's default '/v1' path can override it with this setting.
  basePath: null,

-  initialize: function() {},
+  initialize() {},


I'd either revert this or change it to initialize: () => {},

Yeah, I prefer that style too... Doesn't seem lebab supports it, I think. Will try to see if I can find an eslint autofix...

paulasjes-stripe · 2019-05-06T15:47:07Z

lib/resources/Files.js

        type: 'StreamProcessingError',
-        populate: function(raw) {
+        populate(raw) {


Nit like before, but I prefer populate: (raw) => {

This reverts commit 70fd492.

…eem good. This reverts commit b56f52d.

…backwards-incompatible. This reverts commit 7eba992.

rattrayalex-stripe · 2019-05-08T00:58:08Z

@paulasjes-stripe in 6f2dcb6 I used arrow syntax for more methods; there weren't many places I could safely do this.

I also tried to migrate from self to this, but tests started failing and I aborted before rabbit-holing too deeply. Would appreciate a follow-on PR (no urgency, can happen post-release) finishing this if you're willing to submit one!

r? @ob-stripe

rattrayalex-stripe · 2019-05-08T02:17:51Z

Per IRL,

LGTM

* Add lebab and a script to run it * lebab transform: arrow * lebab transform: arg-rest * lebab transform: arg-spread * lebab transform: obj-method * lebab transform: obj-shorthand * lebab transform: let * lebab transform: template * lebab transform: default-param * lebab transform: destruct-param * lebab transform: includes * Revert "Add lebab and a script to run it" This reverts commit 70fd492. * Revert "lebab transform: destruct-param" because its changes didn't seem good. This reverts commit b56f52d. * Revert "lebab transform: default-param" because it seems dangerous / backwards-incompatible. This reverts commit 7eba992. * Unrelated: mark 8.1 as minimum 8-series version * Add mocha-only script * Use arrows in more places * Loosen some eslint rules I don't love

* Update version to 7 and drop support for Node 4 and Node 5, and Node 7 * Format using Prettier tmp * Alphabetize basic methods * Use 'id' for all single identifier positional arguments * Fix typo * Modernize ES5 to ES6 with lebab (#607) * Add lebab and a script to run it * lebab transform: arrow * lebab transform: arg-rest * lebab transform: arg-spread * lebab transform: obj-method * lebab transform: obj-shorthand * lebab transform: let * lebab transform: template * lebab transform: default-param * lebab transform: destruct-param * lebab transform: includes * Revert "Add lebab and a script to run it" This reverts commit 70fd492. * Revert "lebab transform: destruct-param" because its changes didn't seem good. This reverts commit b56f52d. * Revert "lebab transform: default-param" because it seems dangerous / backwards-incompatible. This reverts commit 7eba992. * Unrelated: mark 8.1 as minimum 8-series version * Add mocha-only script * Use arrows in more places * Loosen some eslint rules I don't love * Remove deprecated methods * Add VSCode and EditorConfig files * Bump dependencies to latest versions * Remove legacy parameter support in invoices.retrieveUpcoming() * Misc. manual formatting (#623) * Misc. manual formatting * Fix some unit tests * Roll back path argument name changes * Misc. manual formatting * Remove "curried" nested resources and manually specified urlParams (#625) * Drop support for optional url params * Delete nested resource files * Remove urlData * Extract urlParams from path instead of manual definition Verified this is no different with: ```js const urlParams = utils.extractUrlParams(spec.path || ''); if ( !(spec.urlParams || []).every((x, i) => urlParams[i] === x) || (spec.urlParams || []).length !== urlParams.length ) { throw Error( 'mismatch' + JSON.stringify(urlParams) + JSON.stringify(spec.urlParams) ); } ``` inside StripeMethod * Remove manually specified urlParams * Add a deprecation error message * Revert "Delete nested resource files" This reverts commit d88a3e7. * Fix nested resources for non-curried urlParams and update tests to demonstrate their use * Refactor makeRequest * Revert "Revert "Delete nested resource files"" This reverts commit e5eccb8. * Extract resources file (to aid with code generation) (#626) * Extract separate resources file (to aid with code generation) * Remove resources that were removed in #625 https://github.com/stripe/stripe-node/pull/625/files#diff-d3dd6c4fd6f915f29d42e4081dc817a8L85 * Update CHANGELOG for 7.0.0 release

rattrayalex-stripe changed the title ~~Modernize ES5 to ES5 with lebab~~ Modernize ES5 to ES6 with lebab May 4, 2019

rattrayalex-stripe force-pushed the ralex/lebab branch from d80c8f5 to f43c604 Compare May 4, 2019 03:55

paulasjes-stripe reviewed May 6, 2019

View reviewed changes

rattrayalex-stripe force-pushed the ralex/lebab branch from 54cd3d4 to 2ef257a Compare May 6, 2019 17:09

rattrayalex-stripe mentioned this pull request May 6, 2019

stripe-node v7.0.0 #606

Merged

13 tasks

rattrayalex-stripe added 16 commits May 7, 2019 17:45

Add lebab and a script to run it

b17570c

lebab transform: arrow

b1fa588

lebab transform: arg-rest

ab372f3

lebab transform: arg-spread

f394b21

lebab transform: obj-method

9b3a65d

lebab transform: obj-shorthand

5c05f95

lebab transform: let

6a7077b

lebab transform: template

162da23

lebab transform: default-param

768b62d

lebab transform: destruct-param

8583868

lebab transform: includes

a7520d9

Revert "Add lebab and a script to run it"

ad18bdb

This reverts commit 70fd492.

Revert "lebab transform: destruct-param" because its changes didn't s…

2d91882

…eem good. This reverts commit b56f52d.

Revert "lebab transform: default-param" because it seems dangerous / …

acea362

…backwards-incompatible. This reverts commit 7eba992.

Unrelated: mark 8.1 as minimum 8-series version

7b6c4fc

Add mocha-only script

37eade8

rattrayalex-stripe force-pushed the ralex/lebab branch from 9066675 to c221f05 Compare May 8, 2019 00:48

rattrayalex-stripe added 2 commits May 7, 2019 17:50

Loosen some eslint rules I don't love

579060e

Use arrows in more places

6f2dcb6

rattrayalex-stripe force-pushed the ralex/lebab branch from c221f05 to 6f2dcb6 Compare May 8, 2019 00:50

rattrayalex-stripe changed the base branch from irace-prettier-formatting to v7.0.0 May 8, 2019 00:51

rattrayalex-stripe assigned ob-stripe May 8, 2019

rattrayalex-stripe merged commit cfd1eb5 into v7.0.0 May 8, 2019

ob-stripe deleted the ralex/lebab branch October 29, 2019 15:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modernize ES5 to ES6 with lebab #607

Modernize ES5 to ES6 with lebab #607

rattrayalex-stripe commented May 4, 2019 •

edited

Loading

rattrayalex-stripe commented May 4, 2019

paulasjes-stripe commented May 6, 2019

irace-stripe commented May 6, 2019

paulasjes-stripe May 6, 2019

rattrayalex-stripe May 6, 2019

paulasjes-stripe May 6, 2019

rattrayalex-stripe commented May 8, 2019

rattrayalex-stripe commented May 8, 2019

	module.exports = function(namespace, resources) {
	return function(stripe) {
	return new ResourceNamespace(stripe, resources);
	};
	};

	const apiVersion = self._stripe.getApiField('version');
	requestData = data;
	const headers = self._defaultHeaders(

Modernize ES5 to ES6 with lebab #607

Modernize ES5 to ES6 with lebab #607

Conversation

rattrayalex-stripe commented May 4, 2019 • edited Loading

rattrayalex-stripe commented May 4, 2019

paulasjes-stripe commented May 6, 2019

irace-stripe commented May 6, 2019

paulasjes-stripe May 6, 2019

Choose a reason for hiding this comment

rattrayalex-stripe May 6, 2019

Choose a reason for hiding this comment

paulasjes-stripe May 6, 2019

Choose a reason for hiding this comment

rattrayalex-stripe commented May 8, 2019

rattrayalex-stripe commented May 8, 2019

rattrayalex-stripe commented May 4, 2019 •

edited

Loading